Skip to content

Improved retrofit timeout handling. #1618

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 28, 2019
Merged

Improved retrofit timeout handling. #1618

merged 1 commit into from
Feb 28, 2019

Conversation

bfg
Copy link
Contributor

@bfg bfg commented Feb 28, 2019

Retrofit Call now uses http client's configured timeout to compute
value for timeout() and blocking execute() method execution timeout.

@bfg
Copy link
Contributor Author

bfg commented Feb 28, 2019

@slandelle, i'm a bit confused about AHC timeouts; as seen from newly created test it's possible to set read timeout to a higher value than request timeout. Which of those values are then being used by AHC instance? If request timeout is always enforced i'll change retrofit timeout computation method.

@slandelle
Copy link
Contributor

If read timeout is longer than request timeout, it will be ignored.

@bfg
Copy link
Contributor Author

bfg commented Feb 28, 2019

@slandelle i've just pushed c3585b1 with updated timeout computation method. Does it make sense to you? If yes, i'm going to squash commit, then you're free to merge.

@slandelle
Copy link
Contributor

No, that doesn't work. read timeout is about stalled response when nothing is received for a long time. Think large file download. You might want a 5s read timeout and a 2 min request timeout.

@bfg
Copy link
Contributor Author

bfg commented Feb 28, 2019

So what would you suggest? timeout() retrofit Call method is used solely by okhttp native client, so it's irrelevant; But there's a possibility that some retrofit call adapters would call execute() in a blocking manner (example: rxjava call adapter initialized with some scheduler) where correct timeout is quite essential; I know that this is not super important, because the whole idea of using AHC with retrofit is to use async retrofitx rxjava call adapter, where timeout handling is done by AHC itself, but nevertheless i would like to improve experience also for users that for some reason initialize call adapters with some other scheduler.

In essence, what i would like to achieve in this PR is to retrieve correct timeout from actual configuration of http client that is going to execute this call; this is improvement, because previously, timeout has been semi-hardcoded in a field and was not configurable by the user.

@slandelle
Copy link
Contributor

Just map on request timeout.

@bfg
Copy link
Contributor Author

bfg commented Feb 28, 2019

👍 i've updated the code, squashed commits; now solely request timeout is used; you're free to merge. And yeah, this time i've tested snapshot with my service :-) Releasing new fix version would be awesome, but it's up to you.

Retrofit `Call` now uses http client's configured request timeout to compute
value for `timeout()` and blocking `execute()` method execution timeout.
@slandelle slandelle merged commit fea53b0 into AsyncHttpClient:master Feb 28, 2019
@bfg bfg deleted the improved_retrofit_timeout_handling branch February 28, 2019 15:25
@slandelle
Copy link
Contributor

@bfg I can't release because your PR breaks javadoc (which is required in order to release on maven central).
Run mvn install javadoc:javadoc -DskipTests in order to reproduce.
It looks like an issue with the latest lombok changes you've introduced.

Could you please fix?

@bfg
Copy link
Contributor Author

bfg commented Feb 28, 2019

Sigh, sure, on it :-)

@bfg
Copy link
Contributor Author

bfg commented Feb 28, 2019

@slandelle it builds successfully for me using maven 3.5/3.6; which maven version are you using?

@bfg
Copy link
Contributor Author

bfg commented Feb 28, 2019

Can you check retrofit_javadoc_issue branch on my fork?

@bfg
Copy link
Contributor Author

bfg commented Feb 28, 2019

I've managed to reproduce javadoc issue. Build works without issues on openjdk 8, but it fails on oracle jdk 8 🤦‍♂️

Java version: 1.8.0_201, vendor: Oracle Corporation, runtime: /opt/java/jdk1.8.0_201/jre

I'll find a way to work around this issue, one way or another :-)

@slandelle slandelle added this to the 2.8.1 milestone May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants